-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Proof of concept for PreviewProvider #24
base: master
Are you sure you want to change the base?
Conversation
|
||
let view = MessageListItemCell() | ||
view.configure(MyViewModel(), theme: .incomingGroupChat) | ||
return view |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UIViewPreview contains always the same view, it can be created once in function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally in that way it will be more self-describing and comments would not be necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm sure there are better options to solve it. I just wanted to quickly check if it's ever possible to use previews in Chat Components (it's a spike) and what are possible limitations so don't treat the current implementation very serious :)
import SwiftUI | ||
import ChatLayout | ||
|
||
#if canImport(SwiftUI) && DEBUG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need DEBUG for PreviewProvider? It should be removed by compiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this at all, what situation would SwiftUI not be importable? Linux?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, we don't need it. You can easily import SwiftUI
for all Apple's platforms so this check is redundant. Another idea could be storing all previews in a separate target like you recently suggested so perhaps we could avoid adding extra macros.
|
||
// MARK: ChatProvider with in-memory storage | ||
|
||
public let testChatProvider: PubNubChatProvider = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I am wrong, but current design supports inheritance so you can make new TestChatProvider class. Is it safe to share testChatProvider between all of previews?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be safe as long as provider remains immutable. It may not be possible though so we could separately create testChatProvider
for each preview.
public var managedObjectId: NSManagedObjectID { return NSManagedObjectID() } | ||
|
||
public func decodedContent<T: Decodable>(from: T.Type) throws -> T { | ||
return try JSONDecoder().decode(T.self, from: Data()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need decision if we using return in single lines or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have been historically including the return, but we can default to the standard when we add SwiftLint and/or SwiftFormat plugin support.
public let testChatProvider: PubNubChatProvider = { | ||
|
||
let chatProvider = PubNubChatProvider( | ||
pubnubProvider: PubNub(configuration: PubNubConfiguration(publishKey: "...", subscribeKey: "...", userId: "JG")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that it is not working but is SDK sending request to API with this keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you interact with your view controller in the Live Preview
mode then it's not possible to avoid these requests. But on the other hand, I don't think that PreviewProvider
is a good place to test message sending or other features that require real communication. I think in this case it's better to involve getting-started
app and run it with your own keys on iOS simulator. It's much more convenient than doing it in the preview window.
In my opinion, previews should be isolated and give you quick answer whether your view (or view controller) looks as expected for various scenarios so the fastest possible way to achieve it should be providing them mock data. Anyway, we could also consider if it's possible to disable any traffic between Components and PubNub server for such previews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats true, preview should not send any requests and I am thinking if it is easy to mock provider, it would help here and in unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still a WIP or should this be reviewed for merging?
} | ||
.previewLayout(.fixed(width: 414, height: 140)) | ||
.previewDisplayName("Light Mode") | ||
.preferredColorScheme(.light) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These three modifiers could be packaged together as a single modifier to simplify reuse when we add similar configurations to other previews. (Same for the following examples)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still a WIP or should this be reviewed for merging?
It's not even a work in progress, but a quick POC instead which I wanted to share with you to make a decision. I can provide a more robust PR converted to Ready for review
if our decision is to have previews for our UIKit views.
|
||
// MARK: ManagedMessageViewModel conformance | ||
|
||
class MyViewModel: ManagedMessageViewModel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there an issue with passing in our PubNubManaged entities directly instead of creating custom View Model implementations with dummy data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a quick try for this POC. I just checked and there are no issues with passing in our PubNubManaged entities.
FYI, I pushed the current state for
PreviewProvider
so you can look at this at any time you want. Just ignore the existing implementation since it's still a POC. We can definitely do it better if we decide to usePreviewProvider